Skip to content

use urls from zos-config if exists #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Eslam-Nawara
Copy link
Contributor

Description

use config managed by environment.go from zos-config and use the ones defined in environment.go as default values

Related Issues

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstring

@Eslam-Nawara Eslam-Nawara requested a review from xmonader June 11, 2025 09:41
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch from 6831098 to 2bade3b Compare June 16, 2025 14:52
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch 2 times, most recently from 2d6d641 to b41608f Compare June 22, 2025 14:27
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch 2 times, most recently from 5655d0d to 13533ba Compare June 22, 2025 15:15
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch from 13533ba to 87e059f Compare June 22, 2025 15:56
@Eslam-Nawara Eslam-Nawara marked this pull request as ready for review June 22, 2025 15:57
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch from 2683a49 to 67517d3 Compare July 7, 2025 10:12
@Eslam-Nawara Eslam-Nawara mentioned this pull request Jul 9, 2025
4 tasks
@Eslam-Nawara Eslam-Nawara force-pushed the load-config-urls-from-zos-config branch from eee4faa to 69773c0 Compare July 23, 2025 13:37
V4HubURL []string `json:"v4_hub_url"`

// we should not be supporting flist url or hub storage from zos-config until we can update them on runtime
// FlistURL string `json:"flist_url"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's have this flisturl configurable just in case we lost our urls again we can boot new machines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to verify that this will actually allow us to deploy a new machine and will not break the node entirely, will start working on that after verifying all changes done are working properly and well tested in all zos types, then will do that

env.GeoipURLs = geoip
}

// flist url and hub storage urls shouldn't listen to changes in config as long as we can't change it at run time.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but if we faced samething when all *.tf were blocked then we will not be able to boot new machines and all those configurable urls will not help unless we can connect to the new storage url

}
return hubStorage
env := environment.MustGet()
return env.HubStorage
Copy link
Collaborator

@ashraffouda ashraffouda Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should return the hubstorage based on if this is v3 or v4 here u r returning one hubstorage all the time

hubStorage   = "zdb://hub.threefold.me:9900"
	hubv4Storage = "zdb://hub.threefold.me:9940

they have diff port
even if u r doing that while creating the env but since u have this getter method make sure u use it every where and do ur logic here otherwise remove this func if not used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

environment.Must get returns hubstorage with v3 if the node is of type v3 and v4 if the node is of type v4, so we don't need to do the check again here

check: https://github.com/threefoldtech/zosbase/pull/33/files#diff-616422b9b983db329c853b3e73274dfdb95b49add7e27bcfd47257923eead1e8R391-R401

@Eslam-Nawara Eslam-Nawara marked this pull request as draft July 28, 2025 09:26
@@ -246,7 +246,7 @@ func isLeastValidNode(ctx context.Context, farmID uint32, substrateGateway *stub
// stop at three and quiet output
err = exec.CommandContext(ctx, "ping", "-c", "3", "-q", ip).Run()
if err != nil {
log.Err(err).Msgf("failed to ping node %d", node.NodeID)
log.Debug().Err(err).Msgf("failed to ping node %d", node.NodeID)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did u change this to debug? this should be visible in the node logs that doesn't booted with debug flag

Copy link
Contributor Author

@Eslam-Nawara Eslam-Nawara Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it didn't have any log level. This error doesn't break the flow, the node logs it and tries to ping another node, so I thought it should be of level debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants